OTLP Exported as default for .NET (Core) #343

Kielek · 2022-02-01T13:05:20Z

Partially Fixes #325
Relates to #324

Changes proposed in this pull request:
Use the OTLP exporter as default for the .NET (Core) and Zipkin for .NET Framework

Tests

CI
Local tests for ConsoleApp, works fine for

.NET 4.6.1 (Zipkin exporter)
.NET Core App 4.1 (otlp)

Works with workaround for

.NET 5.0 (otlp)

Issue to be discussed

https://www.nuget.org/packages/Grpc.Net.Client/2.32.0 used by https://www.nuget.org/packages/OpenTelemetry.Exporter.OpenTelemetryProtocol/1.2.0-rc1 is released for

.NET Standard 2.1
.NET 5.0

Otel-dotnet-inturmenation, for now, is released as .NET 4.6.1 and .NET Core App 3.1.

.NET Core App 3.1 is using .NET Standard 2.1 (deploys Grpc.Net.Client to the tracer-home).

.NET Standard 2.1 of Grpc.Net.Client is not working correctly with .NET 5.0. It leads to following error

info    zapgrpc/zapgrpc.go:174  [transport] transport: loopyWriter.run returning. connection error: desc = "transport is closing"       {"grpc_log": true}

As a workaround, there is a possibility to force load Grpc.Net.Client for .NET 5.0 from the instrumented application. I think that it is not the long-term sollution.

I think that we should consider separate version of otel-intrumentation for .NET 5.0+.

Alternative solution - defaulting to Otel over http/protobuf

Is it possible, and it is working correctly both for .NET Framework 4.6.1, NET Core App 3.1 and .NET 5.0 without any hacks for the end user, but we should consider following things:

defaults are different than prepared by the OTEL .NET SDK,
defaults protocol for otlp exporter should be http/protobuf. The grpc is allowed only when SDK was already released https://github.com/open-telemetry/opentelemetry-specification/blob/ce50e4634efcba8da445cc23523243cb893905cb/specification/protocol/exporter.md?plain=1#L111-L114
most of the code to manage default values is internal API, we should copy (and maintatin!) it to our code call it by reflections https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpTraceExporterHelperExtensions.cs#L61
in the future we should expect more env-variables to manage on our side, see OTEL_EXPORTER_OTLP_TRACES_ENDPOINT, OTEL_EXPORTER_OTLP_METRICS_ENDPOINT, OTEL_EXPORTER_OTLP_TRACES_ENDPOINT, OTEL_EXPORTER_OTLP_METRICS_ENDPOINT, etc.

To reduce the complication on our side we can consider fixing open-telemetry/opentelemetry-dotnet#2857 and waiting for the next RC candidate. If it will cover updates by the Protocol setter it will reduce complication of the code on our side.

Discusion about changing default exporter to HTTP

open-telemetry/opentelemetry-dotnet#2855

it was fixed in #294

Zipkin remains for .NET Framework

make the version compatible with OtelExporter (Grpc.Net.Client)

instead of Grpc.Net.Client for .NET Standard 2.1 from tracer-home

Kielek · 2022-02-02T18:52:12Z

SIG notes: Try to use OTLP over HTTP exporter - check specification and possibility to execute it for all .NET (Core/Framework) versions

Kielek · 2022-02-03T11:14:32Z

@nrcventura, @pjanotti,
I have checked the possibility to use otlp exporter over http/protobuf.
Is it possible, and it is working correctly both for .NET Framework 4.6.1, NET Core App 3.1 and .NET 5.0 without any hacks for the end user, but we should consider following things

defaults are different than prepared by the OTEL .NET SDK,
defaults protocol for otlp exporter should be http/protobuf. The grpc is allowed only when SDK was already released https://github.com/open-telemetry/opentelemetry-specification/blob/ce50e4634efcba8da445cc23523243cb893905cb/specification/protocol/exporter.md?plain=1#L111-L114
most of the code to manage default values is internal API, we should copy (and maintatin!) it to our code call it by reflections https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpTraceExporterHelperExtensions.cs#L61
in the future we should expect more env-variables to manage on our side, see OTEL_EXPORTER_OTLP_TRACES_ENDPOINT, OTEL_EXPORTER_OTLP_METRICS_ENDPOINT, OTEL_EXPORTER_OTLP_TRACES_ENDPOINT, OTEL_EXPORTER_OTLP_METRICS_ENDPOINT, etc.

I do not think that we will have possibility to change the defaults after first official release, so it looks like the long term decision.

Personally, I think that we should follow strategies directly from the .NET OTEL SDK.
The cosnsistency will be better for our endusers what is more, we will have less work to maintain duplicated code.

WIP changes,
https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/compare/main...Kielek:otlp-ver-http-as-default-exporter-net-core?expand=1

When the decision will be made, I will be happy to finish this one or create the new one PR with http/protobud as default.

Kielek · 2022-02-03T19:37:11Z

I will update the first comment to update curent state. It will be esier to follow our options.

pjanotti · 2022-02-03T20:10:06Z

I opened a discussion with SDK for considering a change for the OTLP default from gRPC to HTTP, see open-telemetry/opentelemetry-dotnet#2855

Thanks for the info @Kielek

Kielek · 2022-02-04T06:07:20Z

I will update the first comment to update curent state. It will be esier to follow our options.

First comment updated.

nrcventura · 2022-02-04T19:21:57Z

most of the code to manage default values is internal API, we should copy (and maintatin!) it to our code call it by reflections

Can you explain this a little more? If we only support http/protobuf can we just get away with "hardcoding" some of the public configuration options like users of the SDK might do? I just want to get a better understanding of some of the problems that you are trying to solve.

pellared · 2022-02-07T09:36:09Z

I am good with merging this PR if we describe how OTLP exporter can be used (together with its gotchas) in USAGE.md.

As far as I understand this would require describing that:

gRPC OTLP protocol is not working for .NET Framework. but one can always set the HTTP OTLP protocol
gRPC OTLP protocol requires additional package references on .NET 5 and .NET 6
OTLP endpoint must be manually set if HTTP OTLP protocol is set

We can have a separate PR to switch to HTTP OTLP protocol by default (also for .NET Framework). It would be easier to do it after open-telemetry/opentelemetry-dotnet#2857 is fixed a new version is released. Also, I think that we can keep the support for the gRPC OTLP protocol (with workarounds and constraints described above). But I am also OK to drop support for gRPC completely and just create an issue for it (maybe nobody would need it anyway).

pellared · 2022-02-07T09:21:36Z

samples/ConsoleApp/ConsoleApp.csproj

+    <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="3.0.3" />
+
+    <!-- Hack to make the otel exportert working for .NET 5.0 -->
+    <PackageReference Include="Grpc.Net.Client" Version="2.32.0" Condition="'$(TargetFramework)' == 'net5.0'"/>


If we want this merged then it should be documented in USAGE.md

pellared · 2022-02-07T09:39:32Z

src/OpenTelemetry.ClrProfiler.Managed/Configuration/Settings.cs

-            Exporter = source.GetString(ConfigurationKeys.Exporter);
+            Exporter = source.GetString(ConfigurationKeys.Exporter) ??
+#if NETFRAMEWORK
+                       "zipkin";


alternatively, for.NET Framework, we could still use the OTLP exporter, but set HTTP protocol

Kielek · 2022-02-08T07:26:24Z

Can you explain this a little more? If we only support http/protobuf can we just get away with "hardcoding" some of the public configuration options like users of the SDK might do? I just want to get a better understanding of some of the problems that you are trying to solve.

I think that, I have found pretty cheap alternative to avoid copying whole code related to managing url (lack od auto-extending endpoints with suffix such as /v1/traces or /v1/metrics). I consider is a good tradeoff, especially if open-telemetry/opentelemetry-dotnet#2868 will be merged shortly.

Closing this one, #351 constrains expected changes.

Kielek added 7 commits January 31, 2022 13:16

typo fix

c18adf0

typo fix

fe17b23

remove workaround for #290

acc2ea8

it was fixed in #294

remove redundant usings

f6192b7

make OTLP standard exporter for .NET (Core)

dfe9048

Zipkin remains for .NET Framework

bump Microsoft.Extensions.Logging.Abstractions to 3.0.3 in ConsoleApp

259f112

make the version compatible with OtelExporter (Grpc.Net.Client)

load Grpc.Net.Client for .NET 5.0 from local directory

cc7b2b7

instead of Grpc.Net.Client for .NET Standard 2.1 from tracer-home

Kielek requested a review from a team February 1, 2022 13:05

Kielek mentioned this pull request Feb 1, 2022

bump otel/opentelemetry-collector-contrib from 0.42.0 to 0.43.0 #344

Merged

Kielek changed the title ~~Otlp as default exporter net core~~ OTLP Exported as default for .NET (Core) Feb 1, 2022

pellared reviewed Feb 7, 2022

View reviewed changes

This was referenced Feb 7, 2022

[WIP] Use OTLP exporter by default #324

Closed

Describe OTLP exporter usage #350

Merged

Kielek mentioned this pull request Feb 8, 2022

OTLP over http/protobuf as default exporter #351

Merged

Kielek closed this Feb 8, 2022

Kielek deleted the otlp-as-default-exporter-net-core branch February 8, 2022 07:26

pjanotti mentioned this pull request Mar 2, 2022

Change OTLP exporter default to HTTP when metrics and logs are added open-telemetry/opentelemetry-dotnet#2959

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OTLP Exported as default for .NET (Core) #343

OTLP Exported as default for .NET (Core) #343

Kielek commented Feb 1, 2022 •

edited

Loading

Kielek commented Feb 2, 2022

Kielek commented Feb 3, 2022

Kielek commented Feb 3, 2022

pjanotti commented Feb 3, 2022 •

edited

Loading

Kielek commented Feb 4, 2022

nrcventura commented Feb 4, 2022

pellared commented Feb 7, 2022 •

edited

Loading

pellared Feb 7, 2022

pellared Feb 7, 2022

pellared Feb 7, 2022

Kielek commented Feb 8, 2022

OTLP Exported as default for .NET (Core) #343

OTLP Exported as default for .NET (Core) #343

Conversation

Kielek commented Feb 1, 2022 • edited Loading

Tests

Issue to be discussed

Alternative solution - defaulting to Otel over http/protobuf

Discusion about changing default exporter to HTTP

Kielek commented Feb 2, 2022

Kielek commented Feb 3, 2022

Kielek commented Feb 3, 2022

pjanotti commented Feb 3, 2022 • edited Loading

Kielek commented Feb 4, 2022

nrcventura commented Feb 4, 2022

pellared commented Feb 7, 2022 • edited Loading

pellared Feb 7, 2022

Choose a reason for hiding this comment

pellared Feb 7, 2022

Choose a reason for hiding this comment

pellared Feb 7, 2022

Choose a reason for hiding this comment

Kielek commented Feb 8, 2022

Kielek commented Feb 1, 2022 •

edited

Loading

pjanotti commented Feb 3, 2022 •

edited

Loading

pellared commented Feb 7, 2022 •

edited

Loading